Add support for creating, joining, and parting osu!web rooms via interop #265
Conversation
Mostly a copy-paste from https://github.com/ppy/osu-server-beatmap-submission/blob/82190f3130bae07747fe03a2eadf8cafa81fe401/osu.Server.BeatmapSubmission/Services/LegacyIO.cs, but supports string responses.
Avoid serialising raw details about the exception (e.g. URL) and unwrap
`{ "error": "" }` JSON responses, for instance as returned by the room
creation endpoint.
Client receives:
- `APP_DEBUG=true` -> "422: Unprocessable Entity" (HTTP status code msg)
- `APP_DEBUG=false` -> "beatmaps not found: ..." (osu!web API error msg)
There was a problem hiding this comment.
The base implementation here was pulled from osu-server-beatmap-submission with local adjustments for this server's usage (return value and HubException handling).
| await leaveRoom(state, true); | ||
| await base.CleanUpState(state); | ||
| await leaveRoom(state, true); |
There was a problem hiding this comment.
Meaningless change because the base implementation is empty. I just thought I'd make this consistent with other implementations, seeing as leaveRoom can throw an exception.
| await runLegacyIO(HttpMethod.Delete, $"multiplayer/rooms/{roomId}/users/{userId}"); | ||
| } | ||
|
|
||
| private class CreateRoomRequest : Room |
There was a problem hiding this comment.
mild teeth pain from this inheritance but I guess anything else would probably be more annoying...
i guess this can stay for now maybe but as soon as this is touched for any reason we should probably decouple from Room ASAP
There was a problem hiding this comment.
I agree that this is going to be a potential point of failure if we forget to add any changes to fields/properties going forward. But also, after careful reading it seems this isn't only adding the host user_id but also populating CurrentPlaylistItem.
@smoogipoo might be worth adding some inline commentary as to why this is required. also curious if the PlaylistItem clone is just for sanity of if there's an actual reason that it's required.
There was a problem hiding this comment.
but also populating
CurrentPlaylistItem
might be worth adding some inline commentary as to why this is required
It isn't required. This code used to be part of the base Room model where I attempted to do everything for completed-ness. Removed.
also curious if the
PlaylistItemclone
It isn't a clone, it's converting from a MultiplayerPlaylistItem to a PlaylistItem.
potential point of failure if we forget to add any changes to fields/properties going forward
Am keenly aware of this and see a path where MultiplayerRoom is serialised directly and perhaps via System.Text.Json. I didn't go that deep here to keep things simple (have to deal with enum conversion and all that JSON jankery). Once again I'll reiterate I want to keep the API and spectator models as separate as possible, so I would rather spectator doesn't deal with API models at all.
If it improves things any bit, I can move the ctor into Room. I can't remember why I decided against that in the end but it was an initial path I'd taken.
There was a problem hiding this comment.
If it improves things any bit, I can move the ctor into Room. I can't remember why I decided against that in the end but it was an initial path I'd taken.
As in having everything bar HostUserId populated in the base class but doing the host part here?
There was a problem hiding this comment.
Exactly. My thinking is it'd be harder to miss given there's a CopyFrom() method there, though I'm not sure how long that's going to remain.
There was a problem hiding this comment.
Let's leave it for now, I'm sure we've discussed enough here no one will forget it needs updating 🙄
Co-authored-by: Berkan Diler <[email protected]>
|
Something is going to need to happen to fix the test failures here. Easiest suggestion would be to not have a static ctor in |
|
@smoogipoo I made some naming and minor code structure changes, please check you're okay with them. |
|
Your changes look fine. |
This server needs the ability to manage rooms for matchmaking. Because I intend to use the existing
MultiplayerHubfor matchmaking messages, I took a bit of a detour and made it the single endpoint for creating multiplayer rooms so that the client will no longer need to juggle two endpoints for doing certain things.I see a future where we move playlists, daily challenge, and even the lounge to this server, but I'm taking small meaningful steps because matchmaking is the goal for now.